Skip to content

Conversation

@SylvainJuge
Copy link
Contributor

@SylvainJuge SylvainJuge commented Jul 9, 2025

  • add test application to avoid downloading one in tomcat test
  • move wildfly yaml and documentation to library
  • test from 10.x (~9 year old) to 36.x (latest as of today)

Summary of changes

Network

  • wildfly.network.io metric
    • server attribute renamed to wildfly.server
    • listener attribute renamed to wildfly.listener
    • direction attribute replaced with network.io.direction

Requests / Errors

  • wildfly.request.errorCount metric renamed to wildfly.error.count
  • wildfly.request.requestCount metric renamed to wildfly.request.count
    • consistent with tomcat.request.count
    • server attribute renamed to wildfly.server
    • listener attribute renamed to wildfly.listener
  • wildfly.request.processingTime metric renamed to wildfly.request.duration.sum
    • consistent with tomcat.request.duration.sum
    • server attribute renamed to wildfly.server
    • listener attribute renamed to wildfly.listener

Sessions

  • wildfly.session.expiredSession metric renamed to wildfly.session.expired
    • deployment attribute renamed to wildfly.deployment
  • wildfly.session.rejectedSessions metric renamed to wildfly.session.rejected
    • deployment attribute renamed to wildfly.deployment
  • wildfly.session.sessionsCreated metric renamed to wildfly.session.created
    • deployment attribute renamed to wildfly.deployment
  • wildfly.session.activeSessions metric renamed to wildfly.session.active.count
  • wildfly.session.active.limit metric added

Database connection pool

reusing db.client.connection as part of the namespace to stay close to db client metrics and their attributes in semconv.

Transactions

  • wildfly.db.client.transaction.NumberOfTransactions metric renamed to wildfly.transaction.created
  • wildfly.db.client.rollback.count metric renamed to wildfly.transaction.rollback
    • cause attribute renamed to wildfly.rollback.cause
  • wildfly.transaction.committed metric added to capture the rolled-back transactions
  • wildfly.transaction.count metric added for the current in-flight transactions

@otelbot-java-instrumentation
Copy link
Contributor

🔧 The result from spotlessApply was committed to the PR branch.

@otelbot-java-instrumentation
Copy link
Contributor

🔧 The result from spotlessApply was committed to the PR branch.

@otelbot-java-instrumentation
Copy link
Contributor

🔧 The result from spotlessApply was committed to the PR branch.

@SylvainJuge SylvainJuge marked this pull request as ready for review July 10, 2025 13:40
@SylvainJuge SylvainJuge requested a review from a team as a code owner July 10, 2025 13:40
Copy link
Contributor Author

@SylvainJuge SylvainJuge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be ready for review now.

target.execInContainer("rm", "-fr", "/usr/local/tomcat/webapps/ROOT");
target.execInContainer(
"curl", sampleWebApplicationUrl, "-o", "/usr/local/tomcat/webapps/ROOT.war");
copyTestWebAppToTarget(target, "/usr/local/tomcat/webapps/ROOT.war");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[for reviewer] we use a locally built app instead of downloading one from a remote URL, that removes dependency on network availability and also makes test likely more reliable.

@ValueSource(
strings = {
// keep testing on old and deprecated version for compatibility
"jboss/wildfly:10.1.0.Final",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[for reviewer] in practice we could have tested with earlier versions but some metrics are not available with them. 10.x version is has been released ~9 years ago.

@trask
Copy link
Member

trask commented Jul 10, 2025

cc @PeterF778 @robsunday

Copy link
Contributor

@PeterF778 PeterF778 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@trask trask added this to the v2.18.0 milestone Jul 11, 2025
@SylvainJuge SylvainJuge changed the title align wildfly metrics with semconv (WIP) align wildfly metrics with semconv Jul 11, 2025
metric: created
type: counter
desc: The total number of transactions created
# wildfly.transaction.committed
Copy link
Contributor

@robsunday robsunday Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer having wildfly.transaction.committed.count because it bescribes a data it keeps better than just wildfly.transaction.committed.
Many metrics in this file have .count suffix, but many do not, or even got it removed.
Do you think we could use .count suffix as a standard for counters/updowncounters ? Semconv recommends it for updowncounters anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's something that I really considered doing, however adding the .count to every counter/updowncounter metric seems a bit redundant so I think it's simpler to use it only when needed, for example in the following cases:

  • wildfly.db.client.connection.wait.count adding count helps with the naming as "wait" is not really a noun, also using a similar pattern we could capture the "wait time", for example with wildfly.db.client.connection.wait.duration, in this case it would anticipate a potential future use-case that does not exist yet.
  • wildfly.session.active.count because we also have wildfly.session.active.limit, however for sessions we have wildfly.session.{created,expired,rejected}: there isn't any further breakdown possible, hence it should be fine to define them without .count suffix.

The way I understand the semconv specification here is that we could use .count to remove ambiguity or avoid plural terms, not that we have to use this strategy for every counter.

Copy link
Contributor

@robsunday robsunday Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not 100% convinced, but this is just my personal preference to keep metric names as explicit as possible.
If I see a metric with the name like tomcat.error.count it is obvious that we have number of errors. If I see metric with name tomcat.error it is not obvious what data is there, at least not at the first glance.

And according to the example you brought up:
wildfly.session.active.count, wildfly.session.created, wildfly.session.expired and wildfly.session.rejected share similar category of data, but do not share naming pattern.
I'd really prefer having consitent wildfly.session.{active,created,expired,rejected} or wildfly.session.{active,created,expired,rejected}.count, but I'm not going to hold this PR if nobody else has any objections.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As another point of reference the JVM metrics, which happen to be the only ones that are currently part of semconv tend to only use the .count to indicate the current value, for example we have:

  • jvm.class.loaded
  • jvm.class.count
  • jvm.class.unloaded

And in the case of memory buffers we already have a case where we have "distinct depths" in the same namespace:

  • jvm.buffer.count
  • jvm.buffer.memory.used
  • jvm.buffer.memory.limit

@trask trask removed this from the v2.18.0 milestone Jul 17, 2025
@laurit laurit merged commit 39a68b0 into open-telemetry:main Jul 23, 2025
89 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants